[PowerPC] Check ResNo at end of BitPermutationSelector::Select32 #151429
Conversation
|
@llvm/pr-subscribers-backend-powerpc Author: George Koehler (kernigh) ChangesIf it optimizes away a permutation (rotate all 32 bits left by 0), the result might be from a SDNode with more than one result, such as a load <pre-inc> node. The node replacement assumes result number 0. If it isn't 0, kludge by adding an extra node. Fixes #133507 This isn't the best fix, because I added an extra register move. I wrote in the new test, ; The stw should store POINTER, not VALUE.
; CHECK: lwzu [[VALUE:[0-9]+]], 28([[POINTER:[0-9]+]])
; CHECK: mr [[MOVED:[0-9]+]], [[POINTER]]
; CHECK: stw [[MOVED]], 0({{[0-9]+}})It should lose the Full diff: https://github.com/llvm/llvm-project/pull/151429.diff 2 Files Affected:
diff --git a/llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp b/llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
index 415164fc9e2cb..710662dc107c5 100644
--- a/llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
+++ b/llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp
@@ -2337,6 +2337,11 @@ class BitPermutationSelector {
ANDIVal, ANDISVal), 0);
}
+ // Caller assumes ResNo == 0, but we might have ResNo != 0 after
+ // optimizing away a permutation. Kludge with an extra node.
+ if (Res.getResNo() != 0)
+ return CurDAG->getMachineNode(PPC::OR, dl, MVT::i32, Res, Res);
+
return Res.getNode();
}
diff --git a/llvm/test/CodeGen/PowerPC/lwzu-i48.ll b/llvm/test/CodeGen/PowerPC/lwzu-i48.ll
new file mode 100644
index 0000000000000..8205f5976e876
--- /dev/null
+++ b/llvm/test/CodeGen/PowerPC/lwzu-i48.ll
@@ -0,0 +1,19 @@
+; RUN: llc -mtriple=powerpc-unknown-openbsd < %s | FileCheck %s
+
+; BitPermutationSelector in PPCISelDAGToDAG.cpp was taking the wrong
+; result of a load <pre-inc> after optimizing away a permutation.
+; Here, the big end of i48 %3 was %1 but should be %0.
+
+define i32 @hop(ptr %out, ptr %in) {
+entry:
+ %0 = getelementptr i8, ptr %in, i32 28
+ %1 = load i32, ptr %0, align 4
+ %2 = ptrtoint ptr %0 to i48
+ %3 = shl i48 %2, 16
+ store i48 %3, ptr %out, align 4
+ ret i32 %1
+}
+; The stw should store POINTER, not VALUE.
+; CHECK: lwzu [[VALUE:[0-9]+]], 28([[POINTER:[0-9]+]])
+; CHECK: mr [[MOVED:[0-9]+]], [[POINTER]]
+; CHECK: stw [[MOVED]], 0({{[0-9]+}})
|
clang 19 was miscompiling its own llvm::MergeBasicBlockIntoOnlyPred (Transform/Utils/Local.cpp). A change between clang 18 and 19 exposed a mistake in BitPermutationSelector (PPCISelDAGToDAG.cpp). It optimized away a permutation (rotate all 32 bits left by 0) and got result number 1 from a load pre-inc SDNode, but the node replacement assumed result number 0. Kludge by adding an extra node. llvm/llvm-project#133507 llvm/llvm-project#151429 This isn't the best fix. I will replace it if LLVM commits a different fix. ok miod@
|
My fix is not complete. My patch fixes optimizing away a 32-bit permutation, but not a 64-bit permutation. See define i32 @hop(ptr %out, ptr %in) {
entry:
%0 = getelementptr i8, ptr %in, i32 28
%1 = load i32, ptr %0, align 4
%2 = ptrtoint ptr %0 to i96
%3 = shl i96 %2, 32
store i96 %3, ptr %out, align 8
ret i32 %1
}This emits wrong code, mr 5, 3
lwzu 3, 28(4)
sldi 4, 4, 32
std 3, 0(5)
stw 4, 8(5)
blrThe Notice that t41 = (t43:1 >> 32 << 32) | (t43:1 << 32 >> 32) is equal to t43:1. BitPermutationSelector finds that t41 is t43:1 rotated left by 0, BitPermutationSelector should change t41 to t43:1, but it wrongly changes t41 to t43. |
If BPS optimizes away a permutation (rotate all bits left by 0), the result might be from an SDNode with more than one result, such as a load pre-inc node. The node replacement assumed result number 0. Change it to use an SDValue with the correct result number. Fixes llvm#133507
kernigh
force-pushed
the
kernigh-lwzu-i48
branch
from
b2982a7 to
27e71e5
Compare
|
I forced-push a commit which fixes 64-bit permutation in my last comment, and also loses the extra register move in my 1st comment. |
If it optimizes away a permutation (rotate all 32 bits left by 0), the result might be from a SDNode with more than one result, such as a load node. The node replacement assumes result number 0. If it isn't 0, kludge by adding an extra node.
Fixes #133507
This isn't the best fix, because I added an extra register move. I wrote in the new test,
It should lose the
mrand just dostw [[POINTER]], 0({{[0-9]+}}). It would help me if someone can write a better fix.